Make fwdata api respect nullable return types #2048

myieye · 2025-10-16T08:57:16Z

Resolves #2047

coderabbitai · 2025-10-16T08:57:43Z

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

This PR addresses issue #2047 by modifying FwDataMiniLcmApi to return null instead of throwing when entities are not found. GetEntry, GetSense, and GetExampleSentence now use TryGetObject for safe retrieval. Comprehensive test coverage is added across MiniLcm base classes and multiple implementations to verify null-returning behavior for missing entities.

Changes

Cohort / File(s)	Summary
FwDataMiniLcmApi null-handling updates `backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs`	Modified GetEntry, GetSense, and GetExampleSentence methods to use TryGetObject and return null when entity not found, replacing direct GetObject calls.
MiniLcm test base classes `backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs`, `QueryEntryTestsBase.cs`, `ExampleSentenceTestsBase.cs`	Added new SenseTestsBase abstract class with initialization and test methods for missing/existing sense retrieval. Enhanced QueryEntryTestsBase and ExampleSentenceTestsBase with new test methods verifying Get_Missing* and Get_Existing* behavior.
FwDataMiniLcmBridge SenseTests implementation `backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs`	Created new SenseTests class with ProjectLoaderFixture, decorated with Collection attribute, overriding NewApi to return IMiniLcmApi from fixture.
LcmCrdt SenseTests implementation `backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs`	Created new SenseTests class inheriting from SenseTestsBase with MiniLcmApiFixture, implementing async NewApi and DisposeAsync methods.
Sync consistency tests `backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs`	Added two new test methods verifying cross-silo consistency when example sentences are removed: CrdtEntryMissingTranslationId_FwExampleSentenceRemoved_FullSync and CrdtEntryMissingTranslationId_CrdtExampleSentenceRemoved_FullSync.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The core API changes are straightforward but affect critical read methods with nullable return types. Multiple test classes and implementations are added with consistent patterns across heterogeneous contexts. The spread across multiple files and the importance of null-handling correctness for sync operations warrant careful review.

Possibly related PRs

#1546 — Modifies FwDataMiniLcmApi.cs for null handling of retrieved objects, directly related to same API changes.
#1951 — Modifies FwDataMiniLcmApi.cs and mapping implementations; changes to Get* methods and FromLex* mappings are related to core PR changes.

Suggested reviewers

hahn-kev

Poem

🐰 No more throws when entities flee,
TryGetObject sets us free,
Null returns with graceful care,
Missing data? Null's fair share!
Tests dance merrily left and right,
Safe retrieval, bug-free flight! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "Make fwdata api respect nullable return types" directly and clearly summarizes the main change in the changeset. The title is specific enough to convey the primary objective—modifying the FwData API to handle nullable return types properly by returning null instead of throwing exceptions—and aligns with the core code change in FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs where three API methods (GetEntry, GetSense, GetExampleSentence) were updated to use TryGetObject instead of GetObject.
Linked Issues Check	✅ Passed	The changes directly address the requirements in issue #2047. The core fix is implemented in FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs, where three methods (GetEntry, GetSense, GetExampleSentence) have been modified to use TryGetObject instead of GetObject, returning null when entities are not found rather than throwing exceptions. The supporting changes include new test infrastructure (SenseTestsBase, modifications to ExampleSentenceTestsBase and QueryEntryTestsBase) that verify the null-returning behavior for missing entities, and new test cases in CrdtRepairTests that validate cross-silo consistency when entities are removed, all of which directly support the stated objective of making MiniLcm methods respect nullable return types.
Out of Scope Changes Check	✅ Passed	All changes in this pull request are directly scoped to resolving issue #2047. The primary code changes modify three API methods in FwDataMiniLcmBridge to respect nullable return types, and the supporting changes introduce test infrastructure and test cases to verify the fix works correctly and to set up verification for cross-silo consistency. No unrelated modifications, refactorings, or features are present in the changeset.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

argos-ci · 2025-10-16T08:59:26Z

The latest updates on your projects. Learn more about Argos notifications ↗︎

Awaiting the start of a new Argos build…

github-actions · 2025-10-16T08:59:43Z

UI unit Tests

1 files 45 suites 27s ⏱️
111 tests 111 ✅ 0 💤 0 ❌
160 runs 160 ✅ 0 💤 0 ❌

Results for commit a7b85b1.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs (2)
217-234: Clarify the assertion message on line 229.

The assertion message states "the crdt translation was removed", but the test actually removes the entire example sentence (line 227), not just a translation. The message should reflect that an example sentence was removed, which cascades to remove its translations.

Apply this diff to clarify the message:
-        result.CrdtChanges.Should().Be(1, "the crdt translation was removed");
+        result.CrdtChanges.Should().Be(1, "the crdt example sentence was removed");
277-293: Capture sync result for consistency.

Unlike the similar test at lines 217-234 and the pattern established at line 210, this test doesn't capture the sync result or verify the CrdtChanges count. For consistency and more complete test coverage, consider capturing the result and asserting the expected change count.

Apply this diff to add the assertion:
         // act
         await CrdtApi.DeleteExampleSentence(entryId, senseId, exampleSentenceId);
-        await SyncService.Sync(CrdtApi, FwDataApi);
+        var result = await SyncService.Sync(CrdtApi, FwDataApi);
+        result.CrdtChanges.Should().Be(0, "no crdt changes expected when deleting from crdt side");

         // assert - the fw translation was also removed
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (1)
9-9: Consider making appleId static readonly.

For consistency with SenseTestsBase (line 5-6 in backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs) and since the ID is only used for test setup, consider making this field static readonly.

Apply this diff:
-    private readonly Guid appleId = Guid.NewGuid();
+    private static readonly Guid appleId = Guid.NewGuid();

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d076c6 and a7b85b1.

📒 Files selected for processing (7)

backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs (1 hunks)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (3 hunks)
backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs (2 hunks)
backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs (1 hunks)
backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (2 hunks)
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (3 hunks)
backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (6)

backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs (2)

backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (2)

SenseTestsBase (3-37)

Task (8-21)

backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs (1)

Task (8-11)

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (3)

backend/FwLite/MiniLcm/Models/ExampleSentence.cs (1)

Guid (20-23)

backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (2)

Sense (764-779)

RichString (860-870)

backend/FwLite/MiniLcm/Models/RichString.cs (2)

RichString (11-13)

RichString (87-90)

backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs (2)

backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs (3)

SenseTests (3-19)

Task (7-12)

Task (14-18)

backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (2)

SenseTestsBase (3-37)

Task (8-21)

backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (2)

backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs (1)

MiniLcmTestBase (7-31)

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (1)

Task (9-30)

backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (2)

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (7)

Fact (32-37)

Fact (39-45)

Fact (47-59)

Fact (61-70)

Fact (72-98)

Fact (100-129)

Fact (131-156)

backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (10)

Task (26-29)

Task (95-103)

Task (105-112)

Task (114-118)

Task (120-125)

Task (157-195)

Task (215-233)

Task (235-241)

Task (243-249)

Task (251-256)

backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs (1)

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (8)

Fact (32-37)

Fact (39-45)

Fact (47-59)

Fact (61-70)

Fact (72-98)

Fact (100-129)

Fact (131-156)

Task (9-30)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Build FwHeadless / publish-fw-headless
GitHub Check: Analyze (csharp)
GitHub Check: Build FW Lite and run tests
GitHub Check: frontend-component-unit-tests
GitHub Check: frontend

🔇 Additional comments (10)

backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)

956-957: LGTM! Correctly implements null-returning behavior for missing entities.

The changes to GetEntry, GetSense, and GetExampleSentence properly use TryGetObject instead of GetObject, ensuring these methods return null when entities are not found rather than throwing exceptions. This aligns with the PR objectives and the methods' nullable return types.

Also applies to: 1427-1428, 1554-1555

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (1)

7-7: LGTM! Comprehensive test coverage for null-returning behavior.

The test additions effectively verify that GetExampleSentence returns null for missing entities and returns the correct data for existing ones. The setup in InitializeAsync creates a known example sentence to support the positive test case.

Also applies to: 18-27, 32-45

backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (3)

27-31: LGTM!

Good modification to use a predefined ID for the Apple entry, enabling the new test to verify retrieval by ID.

100-105: LGTM!

The test correctly verifies that GetEntry returns null for a non-existent entry, directly addressing the PR objective.

107-113: LGTM!

The test correctly verifies that GetEntry returns the expected entry and validates its content.

backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs (1)

1-19: LGTM!

The test class correctly implements the fixture pattern for running SenseTestsBase tests against the LcmCrdt implementation. The async lifecycle management is properly handled.

backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs (1)

1-12: LGTM!

The test class correctly uses the xUnit collection fixture pattern to run SenseTestsBase tests against the FwData implementation. This is crucial for verifying that the FwData API now returns null instead of throwing when entities are not found.

backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (3)

8-21: LGTM!

The initialization correctly sets up test data with predefined IDs, enabling predictable testing of sense retrieval behavior across multiple implementations.

23-28: LGTM!

The test correctly verifies that GetSense returns null for a non-existent sense, directly addressing the PR objective to return null instead of throwing.

30-36: LGTM!

The test correctly verifies that GetSense returns the expected sense and validates its content.

Add failing tests

3696ef5

github-actions bot added the 💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related label Oct 16, 2025

coderabbitai bot reviewed Oct 16, 2025

View reviewed changes

Make fwdata api respect nullable return types

2a5ba8a

myieye force-pushed the 2047-fwdata-throws-instead-of-returning-null-when-entity-not-found branch from a7b85b1 to 2a5ba8a Compare October 16, 2025 11:04

myieye merged commit db3ba6b into develop Oct 16, 2025
2 of 9 checks passed

myieye deleted the 2047-fwdata-throws-instead-of-returning-null-when-entity-not-found branch October 16, 2025 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Make fwdata api respect nullable return types #2048

Make fwdata api respect nullable return types #2048

Uh oh!

myieye commented Oct 16, 2025

Uh oh!

coderabbitai bot commented Oct 16, 2025 •

edited

Loading

Review skipped

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

argos-ci bot commented Oct 16, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Oct 16, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Make fwdata api respect nullable return types #2048

Make fwdata api respect nullable return types #2048

Uh oh!

Conversation

myieye commented Oct 16, 2025

Uh oh!

coderabbitai bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

argos-ci bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 16, 2025

UI unit Tests

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai bot commented Oct 16, 2025 •

edited

Loading

argos-ci bot commented Oct 16, 2025 •

edited

Loading